Skip to content

Comments

Read in zero values from cplex#215

Open
willu47 wants to merge 1 commit intomasterfrom
cplex_zero
Open

Read in zero values from cplex#215
willu47 wants to merge 1 commit intomasterfrom
cplex_zero

Conversation

@willu47
Copy link
Member

@willu47 willu47 commented Feb 1, 2024

Reads in zero values from the CPLEX solution file

Description

Removes the piece of code which excluded zero values from the CPLEX solution.

Issue Ticket Number

#214

Documentation

This has not been documented.

@willu47
Copy link
Member Author

willu47 commented Feb 1, 2024

A few things to consider before merging:

  • performance -> how big are the CSV files going to be if they contain zero values?
  • adding a command line flag to make this a user-defined option

@willu47 willu47 requested a review from trevorb1 February 1, 2024 13:00
@trevorb1
Copy link
Member

trevorb1 commented Feb 1, 2024

Hi @willu47! I believe your second bullet point of

adding a command line flag to make this a user-defined option

has been implemented in PR #126! Specifically, the --write_defaults flag. However, I just tried to do a test with it on Simplicity, and the default values were not written out :(

When I run this command

otoole results cplex csv model.sol results-with-zero datafile data.txt config.yaml --write_defaults

The results write out with zeros removed. No helpful debugging logs got printed out either. I tried with cbc as well, and the same thing happened; results write out with the zeros removed.

@trevorb1
Copy link
Member

trevorb1 commented Feb 1, 2024

Ahh.. we are hitting the KeyError exception here. The dubugging statement is just lousy cause it printed out the key REGION rather then something more descriptive. My fault there! haha

otoole/src/otoole/input.py

Lines 260 to 264 in 15bdc0b

if self.write_defaults:
try:
self.input_data = self._expand_defaults(inputs, default_values)
except KeyError as ex:
logger.debug(ex)

@trevorb1 trevorb1 mentioned this pull request Feb 1, 2024
Merged
trevorb1
trevorb1 reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@trevorb1 trevorb1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see PR #216 for my suggestion on how to address reading in zeros from CPLEX. I would vote to leave the removal of zeros in the ReadCplex class because:

  • Be consistent with how other solvers present results
  • This is probably the common desired functionality, so to reduce size of dfs (like you mentioned) when handling data.
  • Use the existing WriteStrategy._expand_defaults() class to achieve the same result

But! I am not opposed to this solution if you (or others) feel we should be presenting unmodified cplex results to the user! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trevorb1 trevorb1 trevorb1 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@willu47 @trevorb1